Support Sparkfun ICM20948 Breakout board (SPI only, I2C TBD)#68
Support Sparkfun ICM20948 Breakout board (SPI only, I2C TBD)#68atinm wants to merge 24 commits intoqqqlab:mainfrom
Conversation
I couldn't get the Adafruit ICM20948 board to support SPI and I didn't want to use I2C. This code is only tested with SPI and 9DOF.
|
Hi @atinm, many thanks for your PRs! I'm currently in holiday mode, and it will take some time before I'll return to my computer keyboard. My comments below are based only on reading your code. I'm not a big fan of copying libs either, but as Arduino IDE has no option to specify a specific lib version I think copying is the way to go to make sure madflight will compile in the future. Could you please trim down the copied sparkfun files: delete the folders: examples, .vscode, img and delete the files in the lib root except licence.md and readme.md For the Arduino IDE we probably need to move the files in SparkFun_ICM-20948_ArduinoLibrary/src one level down to SparkFun_ICM-20948_ArduinoLibrary, or make the util folder a subfolder of src. Otherwise #include "util/ICM_20948_C.h" in src/imu/ICM20948/SparkFun_ICM-20948_ArduinoLibrary/src/ICM_20948.h will fail (I think...) |
|
I am going to try building it exactly as I have it rather than via submodules and see if it works and fix any compile issues. |
|
I just realized why you want to remove the examples etc, it fails your github workflow checks the way it is. I will reorganize the files to fit the madflight style. |
…ADME.md Just use the files necessary for Madflight. Updated Readme to point to the main repository from Sparkfun and for the documentation and license.
|
Removing the folders is required for Arduino, as Arduino compiles all source files in the src folder. At least that is what I thought, I'm surprised to see that for R2040 it compiles... But anyway, these examples (and the other folders) are not required for madflight. If somebody wants to dive into this particular driver, they can look at the sparkfun repo. With ESP32 we get this error: imu.h:35:5: error: 'PinStatus' does not name a type. Apparently PinStatus is not defined for this Arduino flavor. Probably use: Also, I would make has_interrupt_rising_edge a property of ImuGizmo, and not of ImuConfig. I.e. just like has_sensor_fusion, which is also not user configurable, both properties are set by the driver. |
|
@qqqlab actually, not a bad idea removing it from config, but I think I still have to deal with the call to attachInterrupt taking either a PinStatus or an int depending on which board we are using. |
|
Done - moved the interrupt mode to gizmo, default to RISING. |
|
Wow, starts to look really good! Thanks! Apparently I was not clear enough, this is what I was thinking of: if(has_interrupt_rising _edge) { Will work for any arduino flavor, and we get rid of #if defined(ARDUINO_ARCH_MBED) || defined(ARDUINO_ARCH_RP2040). |
|
Done - pass along a bool (still need to change _imu_ll_interrupt_handler's prototype as it doesn't have gizmo available), and removed the #if defined ARDUINO_ARCH_MBED etc. |
|
Great! I think the code looks very good! I still have to study it in more detail and learn some more C++ as your Maybe you could write up some documentation on this PR? As this PR introduces on-chip DMP which involves both IMU and AHR modules it will be helpful to have some guide for the users on how it works. You can post it here and I'll copy/paste it, or create a PR for the madflight-docs repo. |
|
Added comment explaining applyQuatCorrection above it so it is in the code itself and fixed the yaw radian calculation sign to be the same as computeAngles(). |
…ake a while to come up
Since the code is split into .h and .cpp, imu.cpp does not run into register naming conflicts anymore.
|
The code looks good from my perspective. It was a pleasure working with you on this, many thanks! I do not have this sensor, so I can't test in real life. Did you fly with it? Let me know if you're ready to merge, then I'll go ahead. |
|
I might try one more thing by adding a config option for magnetometer so I try it with both on and off (don’t need magnetometer for FPV) as the ICM20948 supports both 9DOF and 6DOF sensor fusion and it should be configurable rather than hardcoded which to use. |
Some IMUs like the ICM20948 allow either 9DOF or 6DOF - allow setting via config whether or not to use magnetometer.
There was a problem hiding this comment.
Let's put this in a new PR - I think this will need some additional considerations...
Some (random) thoughts:
Rather add a new imu_gizmo mf_ICM20948_6DOF than a new parameter. (I try to keep number of parameters as low as possible)
If adding a parameter:
New config params should be added at the end of the array - this allows users to upgrade without eeprom corruption.
I would use value 0 or 1 (not <=0 or 1)
Default value 1 makes more sense to me - use mag if we have it.
Rather name the param and related vars imu_use_mag, and reserve the "has" properties for driver capabilities.
Not consistent with current 9dof drivers...
There was a problem hiding this comment.
Yeah, not consistent with existing drivers - made it use_mag and moved to bottom. Only really used by ICM20948 for now, could be used by MPU9250 if it supports 6DOF. But left others alone to have minimal code changes.
There was a problem hiding this comment.
In the meantime, my quadcopter crashed and burned badly - need to debug it, it stopped listening to IBUS and didn't automatically disarm itself when it lost contact!
There was a problem hiding this comment.
my quadcopter crashed and burned badly
ouch!
There was a problem hiding this comment.
I seemed to have lost some code when I split Icm20948 and ibus code so putting it back - related to calibration and quaternions
Currently only ICM20948 supports actuall using this to enable or disable using its magnetometer, others are just setting has_mag to true or false but, for example, MPU9150 could possibly use this as well if it supports 6DOF. For now left other IMUs as is so as to have minimal code changes.
The quaternions need bias correct, and copying into AHRS. Also added cli commands to show quaternions for IMU and AHRS.
Added calibration of the quaternions for sensor fusion IMUs. Also added pahq and pquat for showing the quaternions from the AHR and the IMU. And finally, added a flipYaw() virtual function to flip the sign of yaw after computeAngles() for IMUs like the ICM20948 that are in LH Android ENU and just quaternion manipulation isn't getting the yaw to the correct sign in computAngles().
|
Hi @atinm, please let me know when you've added all the features you want to have for a PR, and you're confident to merge the PR, then I'll do a final code review. It's more efficient for me than doing many partial reviews, thanks. Specific PR related discussions and questions are always welcome of course. |
|
Hold off while I get my copter flying with this new code! |
2 participants
I have only tested SPI and 9DOF, I didn't try I2C at all. I also couldn't make the Adafruit ICM20948 board do SPI, there seems to be a hardware limitation. I pulled in the whole Sparkfun code with some minor edits (include strings.h, uncomment DMP usage) - it is MIT licensed. I don't really like wholesale copying of external repos and locally I use submodules, but I don't know how that would work with the Arduino IDE and pulling in madflight as a library.
I also had to change computeAngles() for NED and flip the sign so that clockwise around Z would be positive. I am not sure what that would do to other IMUs, it seems to be required to have clockwise be positive.